CORS-4250: AWS: Add the ability to configure throughput on GP3 volumes #2480
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhixson74: This pull request references CORS-4212 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @jhixson74! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/S
|
I see that the linter is failing. For the life of me, I don't know why since every other field in that struct also does not start with the name. Any pointers? ;-) |
|
Pre-existing fields do not meet conventions, please follow the linter so that new content does meet conventions. |
|
Is this feature available in CAPA upstream? |
Yes. |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
4a4d3cf to
ee12153
Compare
|
IIUC, the throughput setting is not yet supported in MAPI (thus this PR is created) right? From OCPSTRAT-2410, it looks like we are not looking to add support MAPI, but rather wait for CAPI migration in OCPSTRAT-1992 (the throughput is already supported there). So, is this change still needed? I might be misunderstanding something... |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
2 times, most recently
from
ab99e54 to
b7238bc
Compare
|
+1 to @tthvo's #2480 (comment) I'm confused. IIUC this PR is adding configuration for MAPI; but MAPI-support is not part of the plan, this is a CAPI-only feature. I think we should just depend on the existing CAPI API and close this PR. @jhixson74 @JoelSpeed can you please clarify? |
We aren't clear yet when we will GA CAPI on OCP. We can avoid adding this to MAPI sure, but that currently restricts control plane usage (as control plane CAPI is further down the road), but it seems like a fairly minor effort to add it IMO and means that conversion between the two types of machine has fewer edge cases |
Agreed. Adding MAPI support seems like the right thing here. |
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
b7238bc to
86385ff
Compare
jhixson74
changed the title
|
@jhixson74: This pull request references CORS-4250 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@jhixson74: This pull request references CORS-4250 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
machine/v1beta1/types_awsprovider.go
Outdated
| // it is not used in requests to create gp2, st1, sc1, or standard volumes. | ||
| // +optional | ||
| Iops *int64 `json:"iops,omitempty"` | ||
| // throughput to provision in MiB/s supported for the volume type. Not applicable to all types. |
There was a problem hiding this comment.
@jhixson74 Are you sure you want to add this to v1beta1 and not v1?
There was a problem hiding this comment.
There is no v1 for this API
| // throughput to provision in MiB/s supported for the volume type. Not applicable to all types. | ||
| // | ||
| // This parameter is valid only for gp3 volumes. | ||
| // Valid Range: Minimum value of 125. Maximum value of 1000. |
There was a problem hiding this comment.
Can it have any value between 125 and 1000?
| // | ||
| // When omitted, this means no opinion, and the platform is left to | ||
| // choose a reasonable default, which is subject to change over time. | ||
| // The current default is 125. |
There was a problem hiding this comment.
Is there value in adding // +default=125? Can its value be modified in a running cluster?
Can we also add a link to the documentation that you have been using for reference?
There was a problem hiding this comment.
We shouldn't add defaults for this API as it's a configuration API. We may want to adjust this default over time so it's better to have nothing persisted and allow this to be defaulted by the controller when acting up on the value
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
machine/v1beta1/types_awsprovider.go
Outdated
| // it is not used in requests to create gp2, st1, sc1, or standard volumes. | ||
| // +optional | ||
| Iops *int64 `json:"iops,omitempty"` | ||
| // throughput to provision in MiB/s supported for the volume type. Not applicable to all types. |
There was a problem hiding this comment.
There is no v1 for this API
| // | ||
| // When omitted, this means no opinion, and the platform is left to | ||
| // choose a reasonable default, which is subject to change over time. | ||
| // The current default is 125. |
There was a problem hiding this comment.
We shouldn't add defaults for this API as it's a configuration API. We may want to adjust this default over time so it's better to have nothing persisted and allow this to be defaulted by the controller when acting up on the value
GP3 volumes have the ability to configure throughput from 125 MiB/s to 1000 MiB/s. This allows the ability to set this at install time in the install-config. https://issues.redhat.com/browse/CORS-4212
jhixson74
force-pushed
the
master_aws_gp3_throughput
branch
from
86385ff to
4194778
Compare
|
New changes are detected. LGTM label has been removed. |
|
@jhixson74: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@jhixson74 API looks good, remind me, did you have a PR to machine-api-operator to implement the validation in the webhook? |
6 participants
GP3 volumes have the ability to configure throughput from 125 MiB/s to 1000 MiB/s. This allows the ability to set this at install time in the install-config.
https://issues.redhat.com/browse/CORS-4212